-
Notifications
You must be signed in to change notification settings - Fork 224
Align Java API with other languages #1560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f7ef1a9 to
592b883
Compare
|
@mcruzdev The patch coverage is failing. Looks like some more unit tests are needed. |
|
LGTM. Can be merged once the patch succeedes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcruzdev thanks a lot for looking into this.
I would like to request changes and suggest some improvements:
- There is another method in the DaprWorkflowClient which uses instance
purgeInstanceI think it should be renamed topurgeWorkflowto stay consistent with other API changes. - We should add a new interface named
WorkflowStatethat should be similar toWorkflowInstanceStatusand we should ensure that instead ofgetInstanceIdwe usegetWorkflowId - Once you have the new interface you should adjust the implementation to ensure that the new methods use the new interface.
- You will also have to add an implementation similar to
DefaultWorkflowInstanceStatusbut forWorkflowStatesomething likeDefaultWorkflowState. - One more thing I think we should adjust Java Docs to use
workflowIdinstead ofinstanceIdso we have a more clean API description.
I know it is a little bit more work, but I think it is totally worth it if we want to align with other SDKs.
Thank you 🙇
7d40e76 to
96eb34e
Compare
|
Hi @artur-ciocanu, I applied all requested changes, let me know if we have something wrong :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcruzdev love it ❤️ ! I have left a tiny comment related to docs. You should make sure that those are aligned with your changes.
Also we need to make sure the build is 🟢.
|
@mcruzdev could you please check the test you added. I see these details in the build logs: And then we have a long list of these: And eventually the task timeouts. |
|
Yes, checking today |
I need to investigate, I just copied the old test using the new API naming. |
|
@mcruzdev I was wondering if you have any updates. Thank you. |
|
Hi @artur-ciocanu, apologies if I missed something. Could you please clarify what’s needed to move this pull request forward? |
|
@mcruzdev there are two checks that are failing, one for Spring Boot 3.4 and the other one for Spring Boot 3.5. As far as I can tell the build for both of these is timing out after 30 mins. The build logs says that DurableTask is trying to connect to Dapr sidecar and that is unavailable on a particular port. Could you please take a look and let me know if you need any help. Thank you. |
3db364c to
9241389
Compare
|
@mcruzdev could you please resolve the conflicts and it should be good to merge. Thank you |
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcruzdev great job! Thanks a lot for all the effort!
|
@dapr/approvers-java-sdk could you please approve. Thank you! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1560 +/- ##
============================================
+ Coverage 76.91% 78.44% +1.53%
- Complexity 1592 1929 +337
============================================
Files 145 216 +71
Lines 4843 5860 +1017
Branches 562 656 +94
============================================
+ Hits 3725 4597 +872
- Misses 821 926 +105
- Partials 297 337 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* Align Java API with other languages Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Update documentation Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Change return type of waitForWorkflowStart method Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> --------- Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> Co-authored-by: artur-ciocanu <artur.ciocanu@gmail.com> Signed-off-by: salaboy <Salaboy@gmail.com>
* Align Java API with other languages Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Update documentation Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Change return type of waitForWorkflowStart method Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> --------- Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> Co-authored-by: artur-ciocanu <artur.ciocanu@gmail.com> Signed-off-by: salaboy <Salaboy@gmail.com>
* Align Java API with other languages Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Update documentation Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Change return type of waitForWorkflowStart method Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> --------- Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> Co-authored-by: artur-ciocanu <artur.ciocanu@gmail.com> Signed-off-by: salaboy <Salaboy@gmail.com>
* Align Java API with other languages Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Update documentation Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Change return type of waitForWorkflowStart method Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> --------- Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> Co-authored-by: artur-ciocanu <artur.ciocanu@gmail.com> Signed-off-by: salaboy <Salaboy@gmail.com>
* Align Java API with other languages Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Update documentation Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> * Change return type of waitForWorkflowStart method Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> --------- Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com> Signed-off-by: artur-ciocanu <artur.ciocanu@gmail.com> Co-authored-by: artur-ciocanu <artur.ciocanu@gmail.com> Signed-off-by: salaboy <Salaboy@gmail.com>
Description
This pull request aims to align the Java SDK Workflow API woth other languages (Python, C#, etc.)
What this pull request does:
waitForInstanceStart,getInstanceStateandwaitForInstanceCompletion.waitForWorkflowStart,getWorkflowStateandwaitForWorkflowCompletion)Issue reference
Closes #1554
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: